Rewrite animation logic #6845

kurkle · 2019-12-17T19:39:49Z

Some small chores 🤣

Minimized size is reduced by 742 bytes, so nothing big there.
This replaces the whole animation logic to target individual properties. Everything can be animated separately, with different duration, delay, easing etc.
Animation configuration can be extended at dataset level, to reduce the need for scriptable animations. Also active, resize keys are added to further reduce need for scripting.
Tooltip has its own animation configuration.

This is greatly inspired by @simonbrunel WIP on datalabels that he showed me many months ago.

Demo

TODO:

Documentation
Add more default properties (at least tension needs to be added)
Samples

benmccann

wow, amazing!!!

benmccann · 2019-12-17T21:31:44Z

src/core/core.animations.js

+		},
+		numbers: {
+			type: 'number',
+			properties: ['x', 'y', 'borderWidth', 'radius']


I think we should add something to animations.md or elsewhere in the new docs about these new options?

We used to figure this out dynamically at runtime, which would reduce the chance for user error forgetting to add something here. Does that have a big performance impact though?

I was hesitant to add property type based fallback, but did not really test the impact on performance.

If that could be disabled in config, then I guess it would be ideal. I think this could be left for a later MR though.

benmccann · 2019-12-17T21:38:51Z

src/controllers/controller.bar.js

 			// all borders are drawn for floating bar
+			/* TODO: float bars border skipping magic


not sure if you want to fix in this one or a follow-up, but just a reminder that this is here

I kind of don't like this kind of things in the code. We don't really know if the user wants to draw all borders in the float bar. It could be default though, but that's kind of hard because float bar does not have its own type.

So this could be "fixed" in many ways and has little to do with the animations.

it could be just documented, so users can add the config

we could separate floatBar to its own type and provide defaults

we could continue overwriting any user config here (not my favorite)

I think I did not do that because those "shared" properties were frozen initially and this caused problems. I ended up removing the freezing though.

Maybe we should just leave it as is for now and handle any change in a separate PR since it's not really related to the animation

benmccann · 2019-12-17T21:42:28Z

src/controllers/controller.line.js

-		for (i = 0, ilen = points.length; i < ilen; ++i) {
-			points[i].pivot(me.chart._animationsDisabled);
+		// Update Points
+		if (meta.visible) {


should we bail out sooner if not visible? i.e. before updating the line

I think some tests failed if I did that, but I could have changed something after that.
This is done to keep the points where they are, when hiding from the legend. (so no movement when shown back, especially when the line is the only one and scale is determining its range automatically)

3 filler related tests fail in that case. I can re-check if either this or #6795 gets merged.

A lot more tests fail now, because options would not be set to line and those are expected in _getMaxOverflow for example.

src/core/core.datasetController.js

benmccann · 2019-12-17T21:53:21Z

src/core/core.animations.js

-			return animation.chart === chart;
-		});
+	/**
+	 * @private


Can you add a little bit of documentation here about how $shared works? That part seems a little complicated

Will do, just commenting here also.

If options are cacheable (= not scripted or indexed), those are shared between elements.

Those shared options are then animated separately. So having one animation object per common point option, like radius. This is mostly why the demo is so fast compared to master where every point's every property is interpolated.

This could be still improved - we could have some of the options shared and some unique to point. But lets leave that for later, if ever.

(talking about point here, because its the element that is usually numerous. applies to bar as well.)

src/core/core.animator.js

src/core/core.controller.js

docs/configuration/tooltip.md

src/controllers/controller.bar.js

etimberg · 2019-12-17T22:21:00Z

src/controllers/controller.bubble.js


 defaults._set('bubble', {
+	animation: {
+		numbers: {
+			properties: ['x', 'y', 'borderWidth', 'radius']


no type field here?

This is extending the default animation, so type is inherited.

kurkle · 2019-12-19T07:07:09Z

Update:

Some docs
Added couple of samples
Made callbacks work (almost) like before
More default animated properties (colors[border, background], base, tension)

benmccann · 2019-12-20T05:03:25Z

@kurkle it looks like there's a file that will need to be rebased

benmccann

Run grep -ri duration docs/ to get a list of some more docs that need to be updated. The migration guide should probably be updated to match those updates as well

docs/configuration/animations.md

benmccann · 2019-12-20T23:12:13Z

src/core/core.animations.js

+			duration: 0
+		},
+		numbers: {
+			type: 'number',


it seems a little redundant to have to specify type when it's already specified as the key. Could we just use the key?

The key name is arbitrary, I just ended up choosing numbers. It could be anything.

I'm actually not sure if this is a good way of specifying defaults. Maybe those should be under defaults, so those can be disabled easily:

animation: { defaults: false, location: { // multiple properties targeted type: 'number', properties: ['x', 'y', 'base'] easing: 'linear', duration: 200 }, dimensions: { // multiple type: 'number', properties: ['width', 'height'], duration: 400, easing: 'easeOutElastic' }, multiKeyBackground: { // single type: 'color', easing: 'linear' } }

But these things that can be altered when people start trying out things (alpha).

docs/configuration/animations.md

kurkle · 2019-12-22T17:29:59Z

Update2:

more docs
another sample (loop)
small tweaks to the active, separated mode and context.active.
- 'active' config is also used when removing hover styles, context.active can be used to differentiate these.

Demo now contains the samples too.

leeoniya · 2019-12-22T17:38:17Z

another sample (loop)

after "Remove Dataset", "Add Data" is busted.

kurkle · 2019-12-22T18:10:37Z

after "Remove Dataset", "Add Data" is busted.

Thats issue #6839

benmccann

i'm really excited about this! it looks pretty close to me. I only have a few minor comments

benmccann · 2019-12-24T03:53:20Z

src/controllers/controller.bar.js

 			// all borders are drawn for floating bar
+			/* TODO: float bars border skipping magic


Maybe we should just leave it as is for now and handle any change in a separate PR since it's not really related to the animation

src/controllers/controller.line.js

src/core/core.animator.js

src/core/core.controller.js

src/core/core.datasetController.js

docs/developers/updates.md

benmccann

lgtm besides @etimberg's one suggestion for the docs. there's a lot here, so I may have missed something, but it seems like a big improvement overall and we will have some time to play with it before v3 is released. thanks for this! it's a really great change!

kurkle · 2019-12-27T08:12:57Z

Added 'none' to api.md.

Some future thoughts:
'hide' and 'show' modes. Needs couple of functions and make Legend use those. hide(datasetIndex, index) and show(datasetIndex, index).

Been also thinking about 'init' and some predefined string values for from / to:

animations: {
  init: {
    y: {
      from: 'base'
    }
  }
}

These predefined strings could be 'top', 'bottom', 'left', 'right' and 'base' (maybe more). These would relate to chartArea.

benmccann · 2019-12-27T16:26:14Z

docs/developers/api.md

-* **duration** (number): Time for the animation of the redraw in milliseconds
-* **lazy** (boolean): If true, the animation can be interrupted by other animations
-* **easing** (string): The animation easing function. See [Animation Easing](../configuration/animations.md) for possible values.
+A `mode` string can be provided to indicate what should be updated and what animation configuration should be used. Core calls this method using any of `undefined`, `'reset'`, `'resize'` or `'active'`. `'none'` is also a supported mode for skipping animations for single update.


What does undefined do? Is it the same as 'none'?

How about 'reset'? Is .update('reset') the same as or related to .reset()?

Its the default/normal update. Uses configure animations.

reset: function() { this._update('reset'); },

etimberg · 2019-12-27T22:19:37Z

After merging I got an email that the build broke

kurkle · 2019-12-27T22:30:04Z

Triggered rebuild and it passed. Not sure what caused this. Its always tarvis through.

kurkle added the type: enhancement label Dec 17, 2019

benmccann reviewed Dec 17, 2019

View reviewed changes

etimberg reviewed Dec 18, 2019

View reviewed changes

kurkle force-pushed the animator branch from db43a90 to 36f69e4 Compare December 19, 2019 06:59

benmccann reviewed Dec 20, 2019

View reviewed changes

kurkle added 3 commits December 22, 2019 17:24

Rewrite animation logic

72c8b07

Review update 1

eb7c2f3

Review update 2

6444410

kurkle force-pushed the animator branch from 36f69e4 to 6444410 Compare December 22, 2019 17:24

benmccann reviewed Dec 24, 2019

View reviewed changes

Review update 3

1858a6d

etimberg reviewed Dec 24, 2019

View reviewed changes

docs/developers/updates.md Show resolved Hide resolved

benmccann previously approved these changes Dec 24, 2019

View reviewed changes

etimberg previously approved these changes Dec 24, 2019

View reviewed changes

Add 'none' to api.md

30d4759

kurkle dismissed stale reviews from etimberg and benmccann via 30d4759 December 27, 2019 07:58

etimberg approved these changes Dec 27, 2019

View reviewed changes

benmccann reviewed Dec 27, 2019

View reviewed changes

benmccann approved these changes Dec 27, 2019

View reviewed changes

etimberg mentioned this pull request Dec 27, 2019

State of Chart.js & V3.0 #6598

Closed

etimberg merged commit b83f64b into chartjs:master Dec 27, 2019

kurkle mentioned this pull request Jan 1, 2020

Point hover animation flicker since 2.9.0 #6614

Closed

This was referenced Jan 9, 2020

Chart not rendering animation completely #6711

Closed

Make certain options animatable #6547

Closed

Animate datasets independently, with different easing and timing options. #6470

Closed

Possible optimization: decouple elements in animation events #5123

Closed

kurkle deleted the animator branch February 19, 2020 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite animation logic #6845

Rewrite animation logic #6845

kurkle commented Dec 17, 2019 •

edited

Loading

benmccann left a comment

benmccann Dec 17, 2019

kurkle Dec 18, 2019

benmccann Dec 17, 2019

kurkle Dec 18, 2019

benmccann Dec 24, 2019

benmccann Dec 17, 2019

kurkle Dec 18, 2019

kurkle Dec 27, 2019

kurkle Dec 28, 2019

benmccann Dec 17, 2019

kurkle Dec 18, 2019

etimberg Dec 17, 2019

kurkle Dec 18, 2019

kurkle commented Dec 19, 2019

benmccann commented Dec 20, 2019

benmccann left a comment

benmccann Dec 20, 2019

kurkle Dec 22, 2019

kurkle commented Dec 22, 2019

leeoniya commented Dec 22, 2019

kurkle commented Dec 22, 2019 •

edited

Loading

benmccann left a comment

benmccann Dec 24, 2019

benmccann left a comment

kurkle commented Dec 27, 2019

benmccann Dec 27, 2019

benmccann Dec 27, 2019

kurkle Dec 27, 2019

kurkle Dec 27, 2019

etimberg commented Dec 27, 2019

kurkle commented Dec 27, 2019

		// all borders are drawn for floating bar
		/* TODO: float bars border skipping magic

Rewrite animation logic #6845

Rewrite animation logic #6845

Conversation

kurkle commented Dec 17, 2019 • edited Loading

benmccann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurkle commented Dec 19, 2019

benmccann commented Dec 20, 2019

benmccann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurkle commented Dec 22, 2019

leeoniya commented Dec 22, 2019

kurkle commented Dec 22, 2019 • edited Loading

benmccann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann left a comment

Choose a reason for hiding this comment

kurkle commented Dec 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg commented Dec 27, 2019

kurkle commented Dec 27, 2019

kurkle commented Dec 17, 2019 •

edited

Loading

kurkle commented Dec 22, 2019 •

edited

Loading